Skip to content

Conversation

dpvc
Copy link
Member

@dpvc dpvc commented Sep 16, 2025

This PR implements a method for enclosing multiple elements when extra ones are included rather than having multiple outlines and backgrounds.

This works by collecting the split nodes into groups by which line they are in (when there are line breaks) and enclosing those collections separately. This is done by inserting a new, temporary node that represents the bounding box of all the elements in each collection. Most of the work is done in the highlighter through the new method encloseNodes(), which does the separation into collections by line, computes the bounding box in each line, and creates and inserts the new node for that bounding box.

How that bounding box node is created and inserted differs in the CHTML and SVG highlighters, and that is handled in the new createEnclosure() method. For HTML elements, and new mjx-bbox element is created with the proper size and offsets, and is prepended to the mjx-container. For SVG, a rect is created and inserted before the first selected element in each line. The creation of the SVG rectangles is abstracted into a createRectangle() method that is used for both the enclosure rectangle as well as the individual background rectangles for the highlighter. If an enclosing rectangle is created, it is used for highlighting as well, so no need to create a highlight rectangle as well.

Selected nodes that are enclosed get marked with a data-mjx-enclosed attribute and their backgrounds are not colored and they don't get the mjx-selected attribute (since the enclosure will get the background and outline).

The unhighlight methods are modified to remove any added enclosures when they become unselected.

The CSS for the CHTML outline is moved from the explorer.ts to the chtml.ts file, to correspond to the CSS in the svg.ts file, though I suppose the CSS from svg.ts could be moved to explorer.ts instead, if that seems better. I also added 'data-' to the sre-highlight attributes, since they are not valid on SVG elements.

@dpvc
Copy link
Member Author

dpvc commented Sep 16, 2025

Oh, I also added a filter to the subtree() method to remove null entries, since there currently are ids in the structure that have no corresponding DOM nodes in the tree. If that gets fixed, the filter should be removed.

@dpvc dpvc requested a review from zorkow September 16, 2025 23:41
@dpvc dpvc added this to the v4.0.1 milestone Sep 16, 2025
Base automatically changed from fix/split_highlighting to develop October 10, 2025 16:17
@zorkow
Copy link
Member

zorkow commented Oct 10, 2025

Oh, I also added a filter to the subtree() method to remove null entries, since there currently are ids in the structure that have no corresponding DOM nodes in the tree. If that gets fixed, the filter should be removed.

I thought that does not happen anymore. Do you have an example expression for this?

@dpvc
Copy link
Member Author

dpvc commented Oct 10, 2025

Do you have an example expression for this?

Well, x_0^1 has data-semantic-structure="(4 (3 0 1) 2)", but there is no node with data-semantic-id="3", and that ends up leaving 3 in the rest array, leading to null after the this.getChild(child) call when the whole expression is selected.

@dpvc
Copy link
Member Author

dpvc commented Oct 10, 2025

That is on the current develop branch of SRE.

Copy link
Member

@zorkow zorkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well with one exception: In the SVG renderer, if there are multiple parts, the fill attribute is only reset/removed on the first of the nodes. The other's retain it.
This does not show up in black on white, however, it shows up, for example, when you put the page into dark mode. Then some elements will remain black after the highlighter has been removed.
The solution is probably to retain the parts in the info structure.

@zorkow
Copy link
Member

zorkow commented Oct 17, 2025

PS: Not sure if this would be a blocker for 4.0.1

@dpvc
Copy link
Member Author

dpvc commented Oct 17, 2025

The solution is probably to retain the parts in the info structure.

That is not necessary. Each part already has its own info structure, and unhighlightNode() is being called for all of them, but the resetting of the foreground color was being skipped for the nodes when there are multiple enclosed nodes. I'm fixing that in a new commit.

… so the cached array is not modified when the enclosure is added
Copy link

codecov bot commented Oct 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.66%. Comparing base (1c2bbef) to head (0cea67e).
⚠️ Report is 76 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1354      +/-   ##
===========================================
- Coverage    86.72%   86.66%   -0.06%     
===========================================
  Files          337      338       +1     
  Lines        84145    84376     +231     
  Branches      3140     4777    +1637     
===========================================
+ Hits         72971    73125     +154     
- Misses       11174    11228      +54     
- Partials         0       23      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dpvc dpvc requested a review from zorkow October 17, 2025 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants